Changes ToolBarDropDownButton from Gtk.MenuButton to Gtk.Dropdown#2092
Changes ToolBarDropDownButton from Gtk.MenuButton to Gtk.Dropdown#2092cameronwhite merged 36 commits intoPintaProject:masterfrom
Conversation
Very buggy, tons of warnings
|
I can't tell you why your code is not working, but there is a working The sample is probably doing what you want and a bit more as the open list is showing more data than the selected item. The logic is not encapsulated in a single class though but this should only be some shifting around of code. How it looks: https://github.com/gircore/gir.core/tree/main/src/Samples/Gtk-4.0/DropDown |
Thanks! I was able to fix it now! Unfortunately the example uses the same "GetSibling" tricks from dev.to's resources, so it didn't help much in the end. I baked the widgets in the ListItem instead and accessed them through the Selected index, which prevented a lot of nulls/warnings. |
|
@cameronwhite I think I fixed everything, but wouldn't hurt to see if I didn't let anything slip. The problems I faced earlier can be seen on the edit history of the Merge Request message, if needed. |
|
Ah yes @kashifsoofi is the author of both. What is wrong about calling next sibling? At least from my point of view the code from @kashifsoofi is less complex as it only needs the liststore as a member and the |
It surely helped to get starting, so I'm very thankful for the article!
Well, every time the binding was updated, it required to walk the widget tree to find the correct elements and account for possible nulls, which gets annoying really fast. Just storing the widgets and querying the position was much simpler and avoided a lot of boilerplate code. Also, it avoids EDIT: wasn't skill issue, Also the 'selected' bind was completely broken for some reason, so as I had to do things manually anyway, it was better to just move everything to logic I control and understand instead of depending on somewhat broken/unintuitive GTK behavior. |
|
In regard to the |
Thanks! That'll come in handy! |
|
From a quick review, I agree with the desire to avoid GetNextSibling() which is brittle / duplicate code that's difficult to maintain if the UI layout is adjusted. But I don't think working around it with extra sidecar data is the best approach either :) IMO a good way to do this is to add a custom widget type for the list items. This makes the update simple since the widget can store references to the updatable child widgets it needs. The GTK list model should be storing the data items (ToolBarItem, or something similar to it, in this case). This would end up looking pretty similar to existing code like the history or layer widgets: |
|
That sounds good. Perhaps we should update the GirCore sample accordingly to have something users can reuse more easily (including having a dedicated class for the dropdown)? |
Btw: I found out just now why my code didn't work before: new Gio.ListStore (ToolBarItem.GetGType ()); // (1) Doesn't work
Gio.ListStore.New (ToolBarItem.GetGType ()); // (2) WorksAs I was using (1), it just didn't work, no matter what I did. @badcel, would it be possible to allow (1) to work the same way as (2)? I imagine many others would face the same problem as I did. I imagine we would just need to add a constructor that called |
They were not updated anywhere, so it's safe to remove them
@cameronwhite Done! I made ToolBarItem a Gtk.Box, which gets passed to the Gio.ListStore. Believe we should be all good now! |
|
I checked the GTK Dropdown code how they do it. As they notify like in the GirCore example I think the code is good enough for me. The additional classes make it a little more verbose but things are more structured at the same time. |
|
Perhaps that |
Agreed, it looks like At the moment I'm updating on Edit: Here's what Matthias Clasen says about this situation: 
Edit 2: Also some feedback from Alice Mikhaylenko: 
So I guess our current implementation is the way to go. |
| { | ||
| Gtk.ListItem item = (Gtk.ListItem) args.Object; | ||
| if (item is null) { return; } | ||
| item.SetChild (selected_box); |
There was a problem hiding this comment.
I'm uneasy about doing non-idiomatic GTK things here like reusing a separate widget.
Could this just create a ToolBarItemWidget? It's basically the same as the list widgets, except you'd need the option for whether to show the label.
A method like ToolBarItemWidget.Bind(toolbarItem, showLabel, showSelected) could work
There was a problem hiding this comment.
The thing is, it is not a ToolbarItemWidget. It is another widget, where there is no "selected icon", there is, in some cases, no label, and there is only an icon. If we hide those items in one place, they will be hidden everywhere, so we cannot reuse them. So we would need to create another widget just for that, which is not worth the code complexity. It's all local anyway, so this wouldn't be exposed.
There was a problem hiding this comment.
If we hide those items in one place, they will be hidden everywhere
I'm not really understanding what you mean here?
My suggestion was that you'd create a separate instance of a ToolbarItemWidget in OnSetupSelectedItem (different than the ones created in OnSetupListItem). So that widget instance can have its options configured appropriately for showing the checkmark / label, and it's a bit more organized than maintaining references to several separate widgets
There was a problem hiding this comment.
If we hide those items in one place, they will be hidden everywhere
I'm not really understanding what you mean here? My suggestion was that you'd create a separate instance of a ToolbarItemWidget in OnSetupSelectedItem (different than the ones created in OnSetupListItem). So that widget instance can have its options configured appropriately for showing the checkmark / label, and it's a bit more organized than maintaining references to several separate widgets
Maybe, but the ToolBarItemWidget has immutable text/icon, we'd need to make it mutable (is it possible, but could conflict with the ToolBarItemWidgets, since the SelectedItem is a single mutable widget and the rest are a bunch of immutable ones). That's why either creating a specific widget for that or doing the widgets as private fields might be a better option.
There was a problem hiding this comment.
I don't have any objections to making the widget mutable - that's actually the intended approach for GTK list views. (e.g. if you have a long list where not all items are visible, the list view might only create the number of widgets required for visible items, and then as you scroll it's just rebinding those widgets to different items in the model - this is the reason for the separate "setup" and "bind" methods in the factory.
But I don't feel too strongly either way since it's conceptually a different type of widget from the list items - it just happens to be similar enough that you could make use of it.
There was a problem hiding this comment.
I don't have any objections to making the widget mutable - that's actually the intended approach for GTK list views. (e.g. if you have a long list where not all items are visible, the list view might only create the number of widgets required for visible items, and then as you scroll it's just rebinding those widgets to different items in the model - this is the reason for the separate "setup" and "bind" methods in the factory.
I think the idea of having the SelectedItem as a model is for added flexibility, I can think of specific cases where you want to show a different widget depending on which item is selected, maybe with additional buttons and actions for some fancy programs. However, for the general case, you probably want an immutable widget where you just use methods to change the text or an icon. Or, as Alice said, most people don't even go there and just avoid custom factories and use the dropdown as-is. You would never create/show more than one of those widgets at the same time tho, so it's a bit disjoint from the usual ListView logic.
But I don't feel too strongly either way since it's conceptually a different type of widget from the list items - it just happens to be similar enough that you could make use of it.
Yeah, it's one of those cases of "we can, but maybe we shouldn't". For example, we would never want a checked item in the SelectedItem, so it would be always there, hidden in the widget tree, taking up memory for something that will never come true. Not that it matters that much, but I think those widgets have different expectations.
| toolbar_item_widgets = []; | ||
| show_label = showLabel; | ||
|
|
||
| widgetList = Gio.ListStore.New (ToolBarItemWidget.GetGType ()); |
There was a problem hiding this comment.
It feels very weird having the "model" list storing the widgets. This should really be a list of the ToolBarItem's
There was a problem hiding this comment.
If we did that, then we'd need to make ToolBarItem inherit from GObject, which I don't see the benefit. That's why I used an empty string list at the start, because we only need the indexes.
There was a problem hiding this comment.
Well, I think the benefit is that the code is less hacky :) Having a model-view widget, but using the widget also as the model, is very odd and goes against the intended use of these GTK apis where widgets are created on demand for the model.
Since the BaseTool implementations interact with ToolBarItem there is some justification for not wanting it to introduce a dependency on GObject, although the better change in the long run would be to isolate the tools from this widget entirely and have something like https://github.com/PintaProject/Pinta/blob/master/Pinta.Core/Classes/ToolOption/IntegerOption.cs for these enum-style options to remove the UI dependency.
With that reasoning I could be okay with leaving this as-is, but it would need a comment explaining the unusual choice.
There was a problem hiding this comment.
Well, I think the benefit is that the code is less hacky :) Having a model-view widget, but using the widget also as the model, is very odd and goes against the intended use of these GTK apis where widgets are created on demand for the model.
Since the BaseTool implementations interact with ToolBarItem there is some justification for not wanting it to introduce a dependency on GObject, although the better change in the long run would be to isolate the tools from this widget entirely and have something like https://github.com/PintaProject/Pinta/blob/master/Pinta.Core/Classes/ToolOption/IntegerOption.cs for these enum-style options to remove the UI dependency.
With that reasoning I could be okay with leaving this as-is, but it would need a comment explaining the unusual choice.
I believe you're right, it can be detrimental to have a widget there. Maybe the StringList option is better (with the comment of avoiding depending on GObject), as I don't use those widgets in the model anyway, and that would make this more explicit.
I like the idea of isolating the tools a lot! Hopefully that can eliminate some of these workarounds. If tools don't depend on the DropDown to retrieve state, then we don't need to store this on the widgets.
There was a problem hiding this comment.
I reverted to StringList to avoid having widgets on the ListStore and to not make ToolBarItem inherit from GObject.
|
@cameronwhite my patch doesn't address disabling items in the dropdown yet, which could be useful in a few instances (like #1965), maybe we could take an ToolBarItem and have an 'enabled' property, which we could use to filter changes to the selected item, for example on the |
|
I think that's best left for another PR to keep this easier to review - will try to have a look later this week! |
This was redundant because 'previous_index' is always initialized at zero. When the selection changes, the SetSelectedIndex function already removes the checkmark on the first item.
Zero-indexed list goes from '0' to 'length - 1'.
|
Thanks, this is much easier to follow 👍 |
3 participants
This allows us to see which element is currently selected in a ToolbarDropDownButton, and also allows us to ship an icon to each list item when the dropdown is opened.
Fix: #1977